Claude finished @iOvergaard's task in 5m 24s —— View job

PR Review

Target: origin/v17/improvement/save-and-publish-take-three · Based on commit: c25740bc · Skipped: 2 files out of 10 total (sdk.gen.ts, types.gen.ts — auto-generated)

Wires the document backoffice to new combined create/update-and-publish endpoints, replacing the previous two-step save-then-publish flow with a single atomic server call; adds logic to re-apply unpublished variant edits after the reload so they remain dirty.

• Modified public API: UmbDocumentServerDataSource (+createAndPublish, +updateAndPublish), UmbDocumentDetailRepository (+createAndPublish, +updateAndPublish), UmbDocumentWorkspaceContext (+performCreateOrUpdateAndPublish, +transferPublishedVariantsToCurrent)
• Other changes: Save-and-publish is now a single atomic server transaction (previously: save → publish → reload). An edited variant that wasn't included in the publish selection now survives the reload as a dirty state.

Suggestions

• document-detail.repository.ts:23: parentUnique has no default value in the repository method, while the underlying data source declares parentUnique: string | null = null. Callers of the repository's createAndPublish must always pass the argument explicitly, which is inconsistent with the data source signature and the analogous base-class create method. Adding = null here makes the API surface consistent. Fix this →

• document-workspace.context.ts:408: performCreateOrUpdateAndPublish is public but intentionally does not update persisted/current data — it requires a caller-side reload() + transferPublishedVariantsToCurrent() to leave the workspace in a valid state. The JSDoc communicates this, but a public method with an invisible post-condition is a footgun for any consumer that calls it standalone. Consider whether protected would be more appropriate (only the single in-package caller exists today), or at minimum add a @throws note spelling out the broken state that results if the follow-up is omitted.

• document-workspace-save-and-publish.context.test.ts:109: The single-call verification test mutates UmbDocumentServerDataSource.prototype at runtime to count invocations. The try/finally cleanup is correct, but the test is white-box: it couples to the internal call path rather than observable server behaviour. A more resilient approach would intercept the MSW request log to assert that exactly one PUT /.../update-and-publish was received and zero PUT /document/:id (plain update) calls were made — that way the assertion survives if the implementation layer is refactored.

Approved with Suggestions for improvement

Good to go, but please carefully consider the importance of the suggestions.

Labels applied: area/frontend, category/ux

Hi,

Is it possible to maintain the current domain split, with all publishing-related methods located in the publishing repository and publishing context? This would mean that methods like createAndPublish would be moved from the DocumentDetailRepository to the UmbDocumentPublishingRepository. I believe this separation of responsibilities aligns better with how package authors may want to extend a workspace with additional functionality, giving us as much flexibility as possible for workspace extensions. Additionally, it will be beneficial if other entities require publishing functionality, as we would only need to "borrow" the Publishing domain across.

Reviewing purely from a testing perspective, I can see the endpoints being called as expected for "create and publish" and "update and publish".

There's an issue with "create and publish" in that after the operation completes there's an immediate and unnecessary display of the "Discard unsaved changes" dialog. If I click "Stay" and then navigate away, there's no secondary display of this dialog, which is correct, so seems there's an issue where this is incorrectly triggered immediately after the create and publish, presumably as it navigates to the "edit" view for the newly created document.

@madsrasmussen Great question!

Agreed on keeping the domain split. One heads-up: moving the createAndPublish/updateAndPublish HTTP methods to UmbDocumentPublishingRepository is easy (the publishing context already has the repo + saveData, and I'll share the request-body mappers).

The catch is the create/update lifecycle around it: setIsNew and the _workspaceEventUnique-stamped UmbEntityUpdatedEvent are protected on the shared base classes, so calling them from the publishing context means exposing them publicly (touches every entity workspace). Are you OK with that?

@AndyButland Interesting finding. I didn't see that myself before, but I will test it out. I think it may be caused by the fact that we now don't overwrite the whole dataset after create/update, because you may not have save-published all variants. Did you have any variants on your page, or any of the known problematic property editors, such as Tiptap with blocks on the document?

I checked - I see this on invariant documents, but it does seem to be property editor related, I've tested with an invariant document that does have an RTE on it, but I haven't populated it with anything.

@iOvergaard I don’t fully remember the entire ceremony around create/update, but it is probably quite complicated. I think there is something interesting in exposing “just enough” so that additional workspace contexts will be able to “prepare” data for a create/save. So maybe we need to introduce new public methods to support this 🤔 I could imagine that other actions might be interested in doing a save/create before their own action and might introduce combined endpoints too.

I was still seeing the same issue @iOvergaard after pulling down the latest - but for one particular document type and not others if I tried to recreate. I found evidence of the problem though:

You see the elements in the values array are in a different order.

I'll give you the diff that looks to fix this in UmbDocumentWorkspaceContext (as would prefer you to review and apply if you agree it makes sense).

-	 * Persisted is reconciled to `savedData` (not the full current data) BEFORE flipping isNew: that flip
-	 * triggers the new->edit redirect, whose navigation guard would otherwise see a transient dirty state
-	 * (the caller's reload reconciles only afterwards) and pop a spurious "Discard unsaved changes" dialog
-	 * (reported by Andy Butland). Using `savedData` keeps published variants clean while edited-but-
-	 * unpublished variants correctly stay dirty, so their edits remain guarded during the pre-reload window.
+	 * Both persisted and current are reconciled to `savedData` BEFORE flipping isNew, so the new->edit
+	 * redirect that flip triggers can never see a dirty state and pop a spurious "Discard unsaved changes"
+	 * dialog. Edited-but-unpublished variants are temporarily absent from the current state until the
+	 * caller's reload + transferPublishedVariantsToCurrent restores them (dirty again).
 	 * @param {UmbDocumentDetailModel} savedData - The data that was sent to the server (constructSaveData)
 	 * @returns {Promise<void>}
 	 * @memberof UmbDocumentWorkspaceContext
 	 */
 	public async finalizeCreate(savedData: UmbDocumentDetailModel): Promise<void> {
 		const parent = this._internal_getCreateUnderParent();
 		if (!parent) throw new Error('Parent is not set');
 
+		// Set persisted AND current to the same data BEFORE flipping isNew: that flip triggers the
+		// new->edit redirect, whose navigation guard compares the two states with an order-sensitive
+		// JSON.stringify. `savedData` is a merge-processed projection of the draft (its values array can
+		// be ordered differently, and resolver-backed editors may rewrite values), so reconciling only
+		// persisted still leaves a spurious mismatch -> "Discard unsaved changes" dialog. 
+		// Edited-but-unpublished variants are restored (and become dirty again) by the
+		// caller's reload + transferPublishedVariantsToCurrent.
 		this._data.setPersisted(savedData);
+		this._data.setCurrent(savedData);
 		this.setIsNew(false);

The test document-workspace-create-and-publish.context.test.ts → "keeps an edited-but-unpublished variant dirty after create-and-publish (no data loss)" will need a look at after this, as it fails after this update and so is perhaps asserting the wrong thing.

• ✅ is not dirty immediately after create-and-publish, before the reload reconciles state — passes (the dialog-window
  guarantee, now holds for all doc types)
• ❌ keeps an edited-but-unpublished variant dirty after create-and-publish (no data loss) — fails at da stays dirty
  (edited but not published): expected false to be true (line 98)

Thanks Andy, that's a good find — the values ordering one is easy to miss. Pushed your fix in 9cbd92b0de4.

Confirmed it's what you said: the dirty check is an order-sensitive JSON.stringify compare, and savedData comes back from the merge controller with its values in a different order than the draft (and resolver editors can rewrite values too), so only reconciling persisted still read as dirty on some doc types.

finalizeCreate now sets both persisted and current to savedData before the flip to edit mode, so the redirect can't see unsaved changes on any doc type. The unpublished variants drop out of current briefly, but the reload + transfer straight after restore them (dirty again) — so no data loss.

You were right about the test as well — it was asserting the intermediate state. Moved it to check the end state after reload + transfer, and renamed it. Green here.

Give it another go on the doc type that was reproducing for you when you get a chance? Want to confirm it's gone on your side too.

Yes, this is working as expected now @iOvergaard. I'll mark as approved from a testing perspective - but if you are still in discussions with @madsrasmussen and @nielslyngsoe on the code/architecture side of things, obviously please wait for merging until you have resolved everything there.

Thanks, @AndyButland.

@madsrasmussen, I implemented your suggestions. Would you check it, please, and let me know what you think?